Omit query analysis error message #282
Conversation
| @@ -147,7 +147,11 @@ class GraphQLController @Inject()( | |||
| } | |||
| }.recover { | |||
| case error: QueryAnalysisError => | |||
| OutgoingQuery(error.resolveError.as[JsObject], None) | |||
| OutgoingQuery( | |||
| Json.obj( | |||
There was a problem hiding this comment.
Do we need to logger.warn or logger.error here? is this frequent or useful?
There was a problem hiding this comment.
This is a validation error, should we still log this?
There was a problem hiding this comment.
Is there still a means to get the error? Usually this is handled by logging the full error with unique token and then returning just that token through the API.
There was a problem hiding this comment.
This is just a temporary solution that we wanted to release before the code freeze tomorrow. We will work on a proper fix later.
There was a problem hiding this comment.
It doesn't have to be fancy. Create a random UUID, log it and the error, then return the UUID. I don't think it's a good idea to obfuscate diagnostics right before the holidays.
There was a problem hiding this comment.
+1 for errorCode, and I feel like message omitted for security is undesired.
There was a problem hiding this comment.
There are other error handlers returning text: 'case error: Exception', 'case error: ErrorWithResolver ', and GraphQLController.exceptionHandler(...). That last one has a TODO that looks security related.
There was a problem hiding this comment.
Considering that this has been long-existing behavior, it's more fine to keep it through the holidays than to jam in a potentially-disruptive change just because of the code freeze.
There was a problem hiding this comment.
I am working on generating random UUID and logging it as @kmcmurtrieca suggested
|
Kudos, SonarCloud Quality Gate passed!
|
JIRA Ticket- https://coursera.atlassian.net/browse/FCORE-562
As part of the security ticket created we want to omit extra information provided by Assembler in case of QueryAnalysisError.